Updating dependencies, updating rollup config file#13
Updating dependencies, updating rollup config file#13LostInBrittany wants to merge 5 commits intodenisemauldin:masterfrom
Conversation
|
Hi @LostInBrittany ! Thanks for the PR. I think the package-lock.json gets generated by npm v5 right? I know there's some debate on whether or not to commit it to source control. What do you think are the advantages of including it comparing to having npm regenerate it? |
|
Well, I prefer to commit the `package-lock.json`, as it's a kind of
guarantee of working condition for the dependencies.
I mean, for me, in `package.json` we are kinda optimistic, we say it should
work with dependency `xyz` version `a.b.c` or greater,
and it should work. But we are only sure that it works on version `a.b.c`.
Commiting the `package-lock.json` is for me a failsafe
way to allow people to test and use the lib without having to deal with any
possible dependency problem.
But that's only my opinion, of course ! :) :)
…On Thu, Apr 26, 2018 at 10:19 PM, Denise Mauldin ***@***.***> wrote:
Hi @LostInBrittany <https://github.com/LostInBrittany> !
Thanks for the PR. I think the package-lock.json gets generated by npm v5
right? I know there's some debate on whether or not to commit it to source
control. What do you think are the advantages of including it comparing to
having npm regenerate it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAsVzLgDZX_x9RRRy5ZWzp4J52zH2SK5ks5tsivGgaJpZM4TlQQL>
.
|
…hemeCategory20 doesn't exist anymore, see https://github.com/d3/d3/blob/master/CHANGES.md#changes-in-d3-50
|
Hi! I've updated the PR as I have changed the default for The reason is that |
|
Hi! There are some errors I need to check before being able to submit you the PR in full confidence, I close it by now and I will reopen it as soon as I will fix them. |
|
Ok, I made all the examples work :) I had a problem with |
|
Hello! When do you think you will have a moment to look at this PR? Sorry to insist, I am using it in an app, and I would prefer dropping the dependency to my fork and put this one :) |
|
@LostInBrittany Ah, I didn't realize you were done trying to fix it. :) Everything is working now? |
|
Oups, sorry, I guess I didn't communicate well, sorry again :(
Yes, everything is working for me, I'am already using it in a web component
( https://www.webcomponents.org/element/LostInBrittany/granite-timeline )
that I'm using in several internal apps right now :)
…On Mon, May 21, 2018 at 7:13 PM, Denise Mauldin ***@***.***> wrote:
@LostInBrittany <https://github.com/LostInBrittany> Ah, I didn't realize
you were done trying to fix it. :) Everything is working now?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAsVzF7inxTMrPuWkL7M29eAJ1IfQgfGks5t0vWzgaJpZM4TlQQL>
.
|
|
|
||
| if (! allowZoom) { | ||
| } | ||
| if (allowZoom) { |
There was a problem hiding this comment.
There needs to be a default zoom. The default zoom is what allows the sideways scrolling in the days_scrollable.html. Why was this change necessary?
There was a problem hiding this comment.
Oh, I commented on that jon the PR thread :)
I had a problem with allowZoom. As written, it only allows zoom is allowZoom(false) is configured.
As I understand it should the the oposite, I have change it. If I am wrong, sorry about that, I guess I don't see the semantics/meaning of the allowZoom method :(
|
@LostInBrittany This PR results in a change in functionality. Please address comment. |
3 participants
Hi! Thanks a lot for having updated this module to D3 version 4+!
I am using it in a D3 v5 project (it works great!), so after having tested it, you have here a PR updating the dependencies (normal, peer and dev). As recent Rollup versions have changed the format of the config file, I've also updated it.
Thanks again for the good job!